Skip to content

Simplify Docker setup and improve local development workflow#140

Open
enko wants to merge 1 commit intomainfrom
claude/review-docker-compose-readme-08Z6V
Open

Simplify Docker setup and improve local development workflow#140
enko wants to merge 1 commit intomainfrom
claude/review-docker-compose-readme-08Z6V

Conversation

@enko
Copy link
Copy Markdown
Member

@enko enko commented Mar 20, 2026

Summary

Refactored the Docker and development setup to provide two distinct workflows: a simplified Docker Compose-based quick start and a faster local development option with hot reload. Updated documentation and npm scripts to reflect these changes.

Key Changes

  • Quick Start: Simplified to use docker compose up -d with clear instructions for database migration and optional seeding
  • Local Development: Added dedicated section for developers who want hot reload without container overhead, using only Docker for the database
  • npm Scripts:
    • Renamed docker:updocker:db (more accurately reflects that it only starts PostgreSQL + PostGIS)
    • Renamed docker:downdocker:db:down (consistent naming)
    • Updated docker:db to only start the postgres service instead of all services
  • Documentation: Updated README.md, AGENTS.md, and docs/development.md to reflect the new workflows and script names

Notable Details

  • The Docker Compose setup now uses an nginx reverse proxy accessible at http://localhost:8080
  • Local development workflow allows running Node.js apps directly on the host machine for faster iteration
  • Both workflows support optional database seeding with sample data

https://claude.ai/code/session_01JTbjLrkLpw9qMsaLup2Kqa

README.md Outdated

```bash
# Prerequisites: Node.js 24+, pnpm 8+
docker compose up -d
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Documentation: Misleading command in Local Development workflow\n\nThis docker compose up -d starts ALL services (including the app containers), which conflicts with the stated intent of running Node.js apps locally for hot reload. Having both containerized and local app instances running simultaneously would cause port conflicts.\n\nSuggested fix:\nbash\npnpm docker:db:up\npnpm install\npnpm migrate\npnpm seed\npnpm dev\n\n\nOr use docker compose up postgres -d directly (which is what pnpm docker:db:up does).

@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 20, 2026

🔍 Automated Code Review

Commit e2acd0f
Reviewed 2026-03-21T00:00:00Z
Status ⚠️ Comments

Findings

Severity Count
🚨 Critical 0
⚠️ Important 0
💡 Suggestion 0

No new issues found in this run. Prior open comments remain (see inline comments on README.md and package.json from earlier runs).

AGENTS.md Compliance

  • ✅ pnpm docker:db:up correctly documented for starting PostgreSQL + PostGIS
  • ✅ pnpm docker:down correctly documented for stopping the docker stack
  • ✅ docker compose (V2) used throughout; deprecated docker-compose (V1) removed
  • ✅ Service names in README (backend, postgres) match docker-compose.yml
  • ✅ Port 8080 (nginx reverse proxy) matches docker-compose.yml configuration
  • ✅ Hot reload claim verified: source volumes and dev commands configured in compose
  • ✅ Docs updated consistently across AGENTS.md, README.md, and docs/development.md
  • ✅ No code changes; linting, type-safety, and test coverage rules not applicable

Summary

This PR is a clean documentation and script-renaming update. The changes correctly reflect the new Docker Compose-first workflow: docker:db:up now only starts the PostgreSQL service, the README quick start uses the full docker compose up -d stack, and all three documentation files are updated consistently. Service names, ports, and hot-reload behavior were verified against docker-compose.yml and all claims are accurate. Two open inline comments from prior runs remain unresolved; no new issues were identified.


🤖 Automated review by Claude Code - View CI Run

Copy link
Copy Markdown

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Found 1 issue to consider. See inline comments.

@enko enko force-pushed the claude/review-docker-compose-readme-08Z6V branch from 462437d to 04b008d Compare March 20, 2026 21:06
package.json Outdated
"docker:up": "docker-compose up -d",
"docker:down": "docker-compose down",
"docker:db:up": "docker compose up postgres -d",
"docker:db:down": "docker compose down",
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Naming: docker:db:down stops all services, not just the database

docker compose down stops and removes all containers in the compose stack (backend, frontend, nginx, postgres), but the docker:db:* namespace implies database-only operations. This is asymmetric with docker:db:up which correctly starts only the postgres service.

Suggested fix — either stop only the postgres service:

"docker:db:down": "docker compose stop postgres"

Or rename to make the broader scope explicit:

"docker:down": "docker compose down"

Copy link
Copy Markdown

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found 1 suggestion to consider. See inline comments for details.

Copy link
Copy Markdown

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No new issues found in this review run. One prior ⚠️ Important concern remains open (see inline comment on README.md regarding docker compose up -d starting all services).

…cripts

Make Docker Compose the default development path in the README with a
note about hot reload support. Remove the separate Local Development
section. Rename docker:up to docker:db:up to clarify they only start
PostgreSQL.
@enko enko force-pushed the claude/review-docker-compose-readme-08Z6V branch from 04b008d to 19e9021 Compare March 21, 2026 08:34
Copy link
Copy Markdown

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No new issues found in this review run. Two prior open comments remain for the author to address (misleading local-dev wording in README.md and script naming semantics in package.json). See the summary comment for full details.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Refactors local development onboarding by emphasizing a Docker Compose “quick start” workflow (with nginx reverse proxy) and updating root scripts/docs to reflect the revised Docker commands.

Changes:

  • Update root npm scripts to use docker compose and add a DB-only startup script (docker:db:up).
  • Refresh developer docs (README, AGENTS, development guide) to reflect the new workflows and access URL (http://localhost:8080).

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
package.json Replaces legacy docker-compose usage and introduces a DB-only docker:db:up script.
docs/development.md Updates documented Docker command to the new DB-only script name.
README.md Revises Quick Start to use docker compose + containerized migrate/seed and notes nginx proxy URL.
AGENTS.md Updates quick command reference for the new Docker script naming.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

"seed": "pnpm --filter backend run seed",
"docker:up": "docker-compose up -d",
"docker:down": "docker-compose down",
"docker:db:up": "docker compose up postgres -d",
Copy link

Copilot AI Mar 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

docker:db:up uses docker compose up postgres -d. Elsewhere in the repo the documented pattern is docker compose up -d postgres (e.g. docs/postgis-address-autocomplete.md), which also matches the canonical CLI syntax. Reordering the args will keep docs consistent and avoid edge-case parsing differences across Docker/Compose versions.

Suggested change
"docker:db:up": "docker compose up postgres -d",
"docker:db:up": "docker compose up -d postgres",

Copilot uses AI. Check for mistakes.
Comment on lines +28 to +29
"docker:db:up": "docker compose up postgres -d",
"docker:down": "docker compose down",
Copy link

Copilot AI Mar 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR description says docker:down was renamed to docker:db:down (and docker:db is mentioned as the “db only” script), but package.json still defines docker:down and introduces docker:db:up. Either update the scripts to match the documented naming, or adjust the PR description/docs to reflect the actual script names to avoid confusion for contributors.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants